Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

pub const MAX_HEIGHT: u8 = 251;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << (MAX_HEIGHT + 1)) - U256::ONE);

Does this work? if so, better to keep it simple

Suggestion:

use ethnum::U256;

#[cfg(test)]
#[path = "types_test.rs"]
pub mod types_test;

const MAX_HEIGHT: u8 = 251;
const MAX_INDEX: U256 = (U256::ONE << MAX_HEIGHT + 1) - U256::ONE;

crates/committer/src/patricia_merkle_tree/types.rs line 29 at r1 (raw file):

    Ord,
)]
pub(crate) struct NodeIndex(pub U256);

This will make the constructor private and enforce using the new function

Suggestion:

pub(crate) struct NodeIndex(U256);

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r1 (raw file):

// Wraps a U256. Maximal possible value is the largest index in a tree of height 251 (2 ^ 252 - 1).
impl NodeIndex {
    pub const BITS: u8 = MAX_HEIGHT + 1;

BIT_LENGTH/BIT_LEN/N_BITS

Code quote:

BITS

crates/committer/src/patricia_merkle_tree/types.rs line 41 at r1 (raw file):

        }
        Self(index)
    }

Maybe implement the trait From from U256

Code quote:

    pub(crate) fn new(index: U256) -> Self {
        if index > *MAX_INDEX {
            panic!("Index is too large.");
        }
        Self(index)
    }

crates/committer/src/patricia_merkle_tree/types.rs line 49 at r1 (raw file):

    pub(crate) fn highest_index() -> NodeIndex {
        NodeIndex::new(*MAX_INDEX)
    }

If it works, better to do it in compile time

Suggestion:

    pub(crate) const ROOT: Self = { Self::from(U256::ONE) };

    pub(crate) const MAX: Self = { Self::from(*MAX_INDEX) };

crates/committer/src/patricia_merkle_tree/types.rs line 84 at r1 (raw file):

        Self(U256::from(1_u8) << tree_height.0) + Self::from(address.0)
    }
}

Why not here?

Code quote:

    pub(crate) fn from_starknet_storage_key(
        key: &StarknetStorageKey,
        tree_height: &TreeHeight,
    ) -> Self {
        Self(U256::from(1_u8) << tree_height.0) + Self::from(key.0)
    }

    pub(crate) fn from_contract_address(
        address: &ContractAddress,
        tree_height: &TreeHeight,
    ) -> Self {
        Self(U256::from(1_u8) << tree_height.0) + Self::from(address.0)
    }
}

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r1 (raw file):

NVM, I see it's consistent with U256

crates/committer/src/patricia_merkle_tree/types.rs line 61 at r1 (raw file):

    /// Returns the number of leading zeroes when represented with Self::BITS bits (252).
    pub(crate) fn leading_zeros(&self) -> u8 {

Suggestion:

    /// Returns the number of leading zeroes when represented with Self::BITS bits.
    pub(crate) fn leading_zeros(&self) -> u8 {

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 62 at r1 (raw file):

    /// Returns the number of leading zeroes when represented with Self::BITS bits (252).
    pub(crate) fn leading_zeros(&self) -> u8 {
        (self.0.leading_zeros() - (U256::BITS - u32::from(Self::BITS)))

Why not keep Self::BITS as u32?

Code quote:

u32::from(Self::BITS)))

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @TzahiTaub and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

No, shift (<<) is not a const function, this is why I use Lazy.

crates/committer/src/patricia_merkle_tree/types.rs line 29 at r1 (raw file):

👍 good pointer
The problem is I do access the field to use its functions. e.g., (first\_leaf.0 & child\_direction\_mask) where the mask is U256. I can use derive_more and turn the mask to NodeIndex, but I'm not sure it's a good idea (as it makes NodeIndex represent numbers and not really just indices in a tree). WDYT?

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r1 (raw file):

Yes, I don't like the names either.

crates/committer/src/patricia_merkle_tree/types.rs line 41 at r1 (raw file):

Instead of new? I can make TryFrom (as not every U256 can be converted) but I prefer to use the ctor with panic than to get an error and handle it everywhere if it makes sense.

crates/committer/src/patricia_merkle_tree/types.rs line 49 at r1 (raw file):

root is fine, max isn't possible inside the NodeIndex impl.

crates/committer/src/patricia_merkle_tree/types.rs line 62 at r1 (raw file):

We can , it's just fits in a u8 and is like the TreeHeight (I don't know why U256 uses u32).

crates/committer/src/patricia_merkle_tree/types.rs line 84 at r1 (raw file):

Why WHAT is not here?

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

Suggestion: something like

const MAX_INDEX: U256 = U256::from_words(!0 >> (255 - MAX_HEIGHT), !0);

Use can move them to NodeIndex and replace (255 - MAX_HEIGHT) with (U256::BITS - Self::BITS)

crates/committer/src/patricia_merkle_tree/types.rs line 29 at r1 (raw file):

You can add a getter:
first_leaf.0 -> first_leaf.index()/first_leaf.num() or something like that

crates/committer/src/patricia_merkle_tree/types.rs line 62 at r1 (raw file):

Because 256 does not fit in 8 bits. Okay, your choice

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

    pub(crate) fn from_contract_address(
        address: &ContractAddress,
        tree_height: &TreeHeight,

The addition is not safe (not going through your new method)
Can we disable the Add impl? are we using it elsewhere?

Suggestion:

Self(U256::ONE << tree_height.0) + Self::from(address.0)

crates/committer/src/patricia_merkle_tree/types.rs line 84 at r1 (raw file):

Using Self::new

Reviewed 3 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 49 at r1 (raw file):

max is impossible also because it's not const (the shift-left operator is not const)

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

@Yoni-Starkware contract addresses, class hashes and storage keys are all less than 2^251 (which is much less than PRIME-1), right?
(otherwise a height of 251 is not enough)
if so, why is the addition unsafe?

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r2 (raw file):

pub const MAX_HEIGHT: u8 = 251;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << (MAX_HEIGHT + 1)) - U256::ONE);

Suggestion:

pub const MAX_HEIGHT: u8 = 251;
const NODE_INDEX_BITS: u8 = MAX_HEIGHT + 1;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << NODE_INDEX_BITS) - U256::ONE);

crates/committer/src/patricia_merkle_tree/types.rs line 35 at r2 (raw file):

// Wraps a U256. Maximal possible value is the largest index in a tree of height 251 (2 ^ 252 - 1).
impl NodeIndex {
    pub const BITS: u8 = MAX_HEIGHT + 1;

delete (see above, use NODE_INDEX_BITS)

crates/committer/src/patricia_merkle_tree/types.rs line 40 at r2 (raw file):

    pub(crate) fn new(index: U256) -> Self {
        if index > *MAX_INDEX {
            panic!("Index is too large.");

Suggestion:

"Index {index} is too large."

crates/committer/src/patricia_merkle_tree/types.rs line 47 at r2 (raw file):

    pub(crate) fn highest_index() -> NodeIndex {
        NodeIndex(*MAX_INDEX)
    }

Suggestion:

    pub(crate) fn highest_index() -> Self {
        Self(*MAX_INDEX)
    }

crates/committer/src/patricia_merkle_tree/types.rs line 100 at r2 (raw file):

        NodeIndex::new(self.0 >> rhs)
    }
}

while you're here

Suggestion:

impl std::ops::Shl<u8> for NodeIndex {
    type Output = Self;

    /// Returns the index of the left descendant (child for rhs=1) of the node.
    fn shl(self, rhs: u8) -> Self::Output {
        Self::new(self.0 << rhs)
    }
}

impl std::ops::Shr<u8> for NodeIndex {
    type Output = Self;

    /// Returns the index of the ancestor (parent for rhs=1) of the node.
    fn shr(self, rhs: u8) -> Self::Output {
        Self::new(self.0 >> rhs)
    }
}

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

I don't understand what is written in your proposal. Is it supposed to be more readable? 😕
And I CAN'T USE << (a non const function) in NodeIndex as we can't define a static there (and we can't use if for const in general)

crates/committer/src/patricia_merkle_tree/types.rs line 29 at r1 (raw file):

Done.

crates/committer/src/patricia_merkle_tree/types.rs line 49 at r1 (raw file):

Yep. I meant it also isn't possible to put a static inside the impl.

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

It's not safe in general (as two NodeIndices can overflow the 252 bit limitations).
As a side note I think we should enforce the hight limitation in TreeHeight as done in NodeIndex (and in other strcuts if there are any).
Not sure what to do here ATM.

crates/committer/src/patricia_merkle_tree/types.rs line 84 at r1 (raw file):

Didn't find it in my search&replace because of the Self usage...

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r2 (raw file):

pub const MAX_HEIGHT: u8 = 251;
static MAX_INDEX: Lazy<U256> = Lazy::new(|| (U256::ONE << (MAX_HEIGHT + 1)) - U256::ONE);

Why? I think it's better for the consts/statics related to a struct to be in it if possible. How about this?

crates/committer/src/patricia_merkle_tree/types.rs line 40 at r2 (raw file):

    pub(crate) fn new(index: U256) -> Self {
        if index > *MAX_INDEX {
            panic!("Index is too large.");

Done.

crates/committer/src/patricia_merkle_tree/types.rs line 47 at r2 (raw file):

    pub(crate) fn highest_index() -> NodeIndex {
        NodeIndex(*MAX_INDEX)
    }

Done.

crates/committer/src/patricia_merkle_tree/types.rs line 100 at r2 (raw file):

Done.

Reviewable status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

This line works, I checked. The >> operates on u128
It is similar to the way U256 defined MAX (pub const MAX: Self = Self([!0; 2]);).
The goal is do avoid Lazy and to define our MAXas const as well.

Reviewable status: 8 of 10 files reviewed, 10 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

Reviewable status: 8 of 10 files reviewed, 8 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

@TzahiTaub do we use this addition anywhere outside this file?

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

    pub(crate) fn index(&self) -> U256 {
        self.0
    }

why not impl From<NodeIndex> for U256? it's safe (the other way around is unsafe)

Code quote:

    /// Returns the index as U256.
    pub(crate) fn index(&self) -> U256 {
        self.0
    }

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

Uhhh, OK then. Changed to u128::MAX, seems less confusing to me (ATM).
You've just searched for const fn from in U256?

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

Yes. Not sure how to find all usages, but there are some, e.g., in updated_skeleton_tree/tree.rs

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

I don't know, it's not really a conversion but a getter. Does this options seem more "rust appropriate"?

Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 13 at r1 (raw file):

I wrote U256::MAX and went to the impl

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

    #[allow(clippy::as_conversions)]
    pub const MAX_INDEX: Self = Self(U256::from_words(
        u128::MAX >> (U256::BITS - Self::BITS as u32),

We don't use as at all (bad practice).
I think it's time to store BITS in u32

Code quote:

Self::BITS as u32

crates/committer/src/patricia_merkle_tree/types.rs line 36 at r4 (raw file):

        u128::MAX >> (U256::BITS - Self::BITS as u32),
        u128::MAX,
    ));

A nice convention from SN API repo.

Suggestion:

impl NodeIndex {
    pub const BITS: u8 = MAX_HEIGHT + 1;
    
    /// [NodeIndex] constant that represents the root index.
    pub const ROOT: Self = Self(U256::ONE);
    
    /// [NodeIndex] constant that represents the largest index in a tree.
    #[allow(clippy::as_conversions)]
    pub const MAX_INDEX: Self = Self(U256::from_words(
        u128::MAX >> (U256::BITS - Self::BITS as u32),
        u128::MAX,
    ));

Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

So I think you should impl these by yourself

    derive_more::Add,
    derive_more::BitAnd,
    derive_more::Mul,
    derive_more::Sub,

(not a big deal)

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

I think so.
NodeIndex -> U256 is a "natural conversion"

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

@Yoni-Starkware u32::from is not const unfortunately; uint conversions in const expressions use as (see blockifier for examples)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 80 at r1 (raw file):

I think Add and Mul are sufficient (the rest can only decrease numbers)

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r3 (raw file):

Alrighty then.

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

This causes a chain reaction to that everything needs to be u32 or converted from u8 (TREE_HEIGHT, bit_length, leading_zeros, maybe more). Why not allow as usage in special casses when you don't lose accuracy (I do need to suppress clippy explicitly so it seems like a reasonable "do if you're sure its OK" thing).

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @TzahiTaub, and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 96 at r6 (raw file):

        Self::new(self.0 * rhs.0)
    }
}

does U256 panic on overflow?
I think we'll want to panic.

1. If U256 already panics then @Yoni-Starkware I think using the derive was just as good
2. If not, please panic explicitly

Code quote:

impl std::ops::Add for NodeIndex {
    type Output = Self;

    fn add(self, rhs: Self) -> Self {
        Self::new(self.0 + rhs.0)
    }
}

impl std::ops::Mul for NodeIndex {
    type Output = Self;

    fn mul(self, rhs: Self) -> Self {
        Self::new(self.0 * rhs.0)
    }
}

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/compute_updated_skeleton_tree.rs line 57 at r6 (raw file):

        let child_direction_mask = U256::ONE << (root_height.0 - 1);
        (U256::from(first_leaf) & child_direction_mask)
            != (U256::from(*last_leaf) & child_direction_mask)

or not, your call, non-blocking

Suggestion:

        let child_direction_mask = U256::ONE << (root_height.0 - 1);
        (child_direction_mask & first_leaf.into())
            != (child_direction_mask & last_leaf.into())

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 96 at r6 (raw file):

It does, thats why Sub is fine, but we want to avoid an overflow of NodeIndex (252 bits) and not U256.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @Yoni-Starkware)

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/compute_updated_skeleton_tree.rs line 57 at r6 (raw file):

Tried that before, didn't work (it needs an explicit type in any case)

Reviewed 3 of 7 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 34 at r4 (raw file):

Okay, that sounds reasonable.

crates/committer/src/patricia_merkle_tree/types.rs line 96 at r6 (raw file):

Yeah, what @TzahiTaub said

Reviewed all commit messages.
Reviewable status: 6 of 10 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

Reviewed 2 of 6 files at r2, 1 of 2 files at r4, 3 of 4 files at r6, 3 of 4 files at r7.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

Reviewed 1 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 8 at r7 (raw file):

use ethnum::U256;

use super::node_data::inner_node::EdgePath;

don't use super outside of tests

Code quote:

use super::node_data::inner_node::EdgePath;

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r7 (raw file):

    ) -> NodeIndex {
        let PathToBottom { path, length } = path_to_bottom;
        (index << length.0) + (*path).into()

make this explicit

Code quote:

(*path).into()

crates/committer/src/patricia_merkle_tree/types.rs line 143 at r7 (raw file):

        Self::new(U256::from_be_bytes(value.0.to_bytes_be()))
    }
}

remove (see above)

Code quote:

impl From<EdgePath> for NodeIndex {
    fn from(value: EdgePath) -> Self {
        Self::new(U256::from_be_bytes(value.0.to_bytes_be()))
    }
}

Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 8 at r7 (raw file):

Done.

crates/committer/src/patricia_merkle_tree/types.rs line 52 at r7 (raw file):

Done.

crates/committer/src/patricia_merkle_tree/types.rs line 143 at r7 (raw file):

Done.

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @TzahiTaub)

crates/committer/src/patricia_merkle_tree/types.rs line 50 at r8 (raw file):

    ) -> NodeIndex {
        let PathToBottom { path, length } = path_to_bottom;
        (index << length.0) + Self::new(U256::from_be_bytes(path.0.to_bytes_be()))

Suggestion:

Self::from_felt_value(path.0)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)

crates/committer/src/patricia_merkle_tree/types.rs line 50 at r8 (raw file):

    ) -> NodeIndex {
        let PathToBottom { path, length } = path_to_bottom;
        (index << length.0) + Self::new(U256::from_be_bytes(path.0.to_bytes_be()))

Done.

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)